-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pre-populate required arguments from request body #4827
Conversation
This looks bad:
|
So, the problem here is that the client is reading off one arg and then forwarding that to the server. However, I've replaced |
So, the question here is, should we check the arguments on the client? Probably but, in that case, we're going to be splitting them (passing one via the query string and the rest via the body). |
This actually seems reasonable. It moves us towards having the requirement that all required arguments are specified via the query string, and the body is only used for optional/variadic args |
Wait up, i must have missed this in the CR. We should just replace files with the next file. |
|
ce316e5
to
9837f0e
Compare
This way, we can always assume that indexing a required argument works. Also: * test that the command tree doesn't have any obvious bugs (duplicate options, arguments in the wrong order, etc). * simplify the usage ParseBodyArgs. * remove unnecessary check in the get command. fixes #4823 License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>
for both online and offline mode (tests both the cli and the api) License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>
grep "not enough arugments provided" curl_out | ||
|
||
|
||
grep "argument \"ipfs-path\" is required" curl_out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats a much better error message.
pre-populate required arguments from request body This commit was moved from ipfs/kubo@4bdbe1a
This way, we can always assume that indexing a required argument works.
Also:
fixes #4823